Several updates to ValueTask #16098

stephentoub · 2018-01-30T16:05:57Z

Make ValueTask code more easily shared with corefx

Remove superfluous use of tuples that cause complications with corefx's netstandard1.0 build
Move CoreCLR-specific API usage into a partial struct

With those changes, we'll be able to fully share the ValueTask code and associated CompilerServices code, other than the three methods in the .CoreCLR.cs file, and can delete corefx's duplicate implementation.

Make ValueTask awaiters readonly structs

They previously weren't flagged because they had a non-readonly field. That field is the wrapped ValueTask, and it wasn't readonly because doing so would have resulted in copies being made when invoking methods on ValueTask. But now that ValueTask itself is readonly, we can make the field and then the struct readonly.

Remove defunct CreateAsyncMethodBuilder method

This is a holdover from a beta release of a C# compiler. It's not needed, and we agreed in API review that even though it did ship in the ref for the System.Threading.Tasks.Extensions.dll, the chances of it actually breaking anyone are close to zero, and the workaround if it does is to simply replace usage of it with default.

cc: @jkotas, @benaadams

The _value field wasn't previously readonly, because method calls were made on the ValueTask, resulting in a copy. But now that ValueTask is a readonly struct, the field can be readonly, and these structs can then also be readonly.

It was superfluous and made sharing the code with corefx more challenging due to the netstandard1.0 build of System.Threading.Tasks.Extensions in corefx.

stephentoub · 2018-01-30T16:22:40Z

@dotnet-bot test this please (most legs didn't start)

stephentoub · 2018-01-30T16:23:17Z

@dotnet/dnceng, is there an issue with legs getting started in coreclr?

mmitche · 2018-01-30T16:24:49Z

@stephentoub I think so...investigating.

mmitche · 2018-01-30T16:55:53Z

@dotnet-bot test this please

jkotas · 2018-01-30T17:10:32Z

src/mscorlib/src/System/Threading/Tasks/ValueTask.CoreCLR.cs

+
+        private bool IsTaskCompletedSuccessfully => _task.IsCompletedSuccessfully;
+
+        private Task<TResult> GetTaskForResult() => AsyncTaskMethodBuilder<TResult>.GetTaskForResult(_result);


Are these extra layers going to regress perf? Note that the JIT is not great at inlining a complex generic constructs like these.

I'll check.

If it ends up being a problem, I expect the best recourse would be to just ifdef it. Is there a compilation constant we use for this kind of thing? #if CORECLR?

I think we should be using #if netstandard for the less efficient portable version since the portable version is the odd one. Everything else will want to use the more efficient version - CORECLR, CORERT, MONO.

jkotas · 2018-01-30T17:11:35Z

src/mscorlib/src/System/Threading/Tasks/ValueTask.CoreCLR.cs

+{
+    readonly partial struct ValueTask<TResult>
+    {
+        private static void ThrowNullTaskException() => ThrowHelper.ThrowArgumentNullException(ExceptionArgument.task);


It would be nice to have add ThrowHelper to the CoreFX package instead so that a copy of this method is not stamped into each ValueTask instantiaion.

Is there an issue for that, or anyone working on it?

I do not think so.

Ok, I'll open an issue. In the meantime I can just use ifdefs if you prefer.

I do not understand why you would not want to just do it as part this set of changes ... it just requires extra change in your CoreFX PR.

What about corert?

Move to internal non-generic static class in meantime?

Oh, were you suggesting adding a ThrowHelper to corefx rather than moving coreclr's implementation to shared? I thought you were suggesting moving it to shared. If you're just talking about a simple equivalent in corefx, sure, that's easy.

Right, I am just talking about a simple local equivalent for ValueTuple in corefx. I do not think sharing ThrowHelper.cs makes sense.

jkotas · 2018-01-30T17:13:17Z

src/mscorlib/shared/System/Runtime/CompilerServices/ConfiguredValueTaskAwaitable.cs

@@ -74,7 +74,11 @@ internal ConfiguredValueTaskAwaiter(ValueTask<TResult> value, bool continueOnCap

            /// <summary>Gets the task underlying the incomplete <see cref="_value"/>.</summary>
            /// <remarks>This method is used when awaiting and IsCompleted returned false; thus we expect the value task to be wrapping a non-null task.</remarks>
-            (Task task, bool continueOnCapturedContext) IConfiguredValueTaskAwaiter.GetTask() => (_value.AsTaskExpectNonNull(), _continueOnCapturedContext);


I like the change to not use ValueTuple here. It will improve .NET Core as well.

ValueTuples are pretty expensive types with large footprint, unfortunately. Not as bad as Linq, but bad enough.

This makes me sad

stephentoub · 2018-01-30T21:08:00Z

@dotnet-bot test Tizen armel Cross Checked Innerloop Build and Test please

Several updates to ValueTask Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

Several updates to ValueTask Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>

stephentoub added 3 commits January 30, 2018 10:12

Remove defunct CreateAsyncMethodBuilder from ValueTask

f748d23

Make ValueTaskAwaiters readonly structs

1e96139

The _value field wasn't previously readonly, because method calls were made on the ValueTask, resulting in a copy. But now that ValueTask is a readonly struct, the field can be readonly, and these structs can then also be readonly.

Remove ValueTuple usage from ConfiguredValueTaskAwaitable

eb234ae

It was superfluous and made sharing the code with corefx more challenging due to the netstandard1.0 build of System.Threading.Tasks.Extensions in corefx.

stephentoub mentioned this pull request Jan 30, 2018

Use ValueTask shared from corelib dotnet/corefx#26636

Merged

jkotas reviewed Jan 30, 2018

View reviewed changes

Make ValueTask shareable with corefx

de70b09

stephentoub force-pushed the valuetaskshared branch from 675a5a1 to de70b09 Compare January 30, 2018 19:42

jkotas approved these changes Jan 30, 2018

View reviewed changes

benaadams approved these changes Jan 30, 2018

View reviewed changes

stephentoub merged commit 0099e62 into dotnet:master Jan 30, 2018

stephentoub deleted the valuetaskshared branch January 30, 2018 22:47

dotnet-bot pushed a commit to dotnet/corert that referenced this pull request Jan 30, 2018

Merge pull request dotnet/coreclr#16098 from stephentoub/valuetaskshared

067af94

Several updates to ValueTask Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

jkotas pushed a commit to dotnet/corert that referenced this pull request Jan 31, 2018

Merge pull request dotnet/coreclr#16098 from stephentoub/valuetaskshared

732a7b6

Several updates to ValueTask Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

dotnet-bot pushed a commit to dotnet/corefx that referenced this pull request Jan 31, 2018

Merge pull request dotnet/coreclr#16098 from stephentoub/valuetaskshared

ceb889c

Several updates to ValueTask Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>

jkotas pushed a commit to dotnet/corefx that referenced this pull request Feb 1, 2018

Merge pull request dotnet/coreclr#16098 from stephentoub/valuetaskshared

aa2eae2

Several updates to ValueTask Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Several updates to ValueTask #16098

Several updates to ValueTask #16098

stephentoub commented Jan 30, 2018

stephentoub commented Jan 30, 2018

stephentoub commented Jan 30, 2018

mmitche commented Jan 30, 2018

mmitche commented Jan 30, 2018

jkotas Jan 30, 2018

stephentoub Jan 30, 2018

stephentoub Jan 30, 2018 •

edited

Loading

jkotas Jan 30, 2018

jkotas Jan 30, 2018

stephentoub Jan 30, 2018

jkotas Jan 30, 2018

stephentoub Jan 30, 2018

jkotas Jan 30, 2018 •

edited

Loading

stephentoub Jan 30, 2018

benaadams Jan 30, 2018

stephentoub Jan 30, 2018

jkotas Jan 30, 2018

jkotas Jan 30, 2018

davidfowl Jan 30, 2018

stephentoub commented Jan 30, 2018


		private bool IsTaskCompletedSuccessfully => _task.IsCompletedSuccessfully;

		private Task<TResult> GetTaskForResult() => AsyncTaskMethodBuilder<TResult>.GetTaskForResult(_result);

Several updates to ValueTask #16098

Several updates to ValueTask #16098

Conversation

stephentoub commented Jan 30, 2018

stephentoub commented Jan 30, 2018

stephentoub commented Jan 30, 2018

mmitche commented Jan 30, 2018

mmitche commented Jan 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub Jan 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas Jan 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub commented Jan 30, 2018

stephentoub Jan 30, 2018 •

edited

Loading

jkotas Jan 30, 2018 •

edited

Loading